[#3600] Add new Bundler/DuplicatedGem cop
#3638
Conversation
| private | ||
|
|
||
| def from_gemfile?(processed_source) | ||
| File.basename(processed_source.path) == 'Gemfile' |
There was a problem hiding this comment.
You can just use configuration to make a cop apply only to the Gemfile. Seems cleaner to me.
There was a problem hiding this comment.
Also worth noting that there are some other valid names, such as gems.rb
There was a problem hiding this comment.
Agreed, it would also be beneficial to able to scan things such as the Gemfile.local in the RuboCop project. Other gems follow similar structures for testing against multiple version of certain gems such as Rails. Bullet has a good example of this.
| def investigate(processed_source) | ||
| return unless from_gemfile?(processed_source) | ||
|
|
||
| gem_requirements(processed_source.ast) |
There was a problem hiding this comment.
Instantiating the hash here, and mutating it by reference from other methods makes this code very hard for me to follow.
| File.basename(processed_source.path) == 'Gemfile' | ||
| end | ||
|
|
||
| def gem_requirements(ast) |
There was a problem hiding this comment.
Maybe this should be named #gem_declarations? When saying "requirements", it makes me think about the version constraints. 😀
There was a problem hiding this comment.
Requirements definitely seeped in from reading the bundler docs 😄
| ast.each_descendant.select { |e| gem_method?(e) } | ||
| end | ||
|
|
||
| def extract_gem_name(gem_method_node) |
There was a problem hiding this comment.
I think the extract_ prefix doesn't add anything to this method name. WDYT? 😀
| end | ||
|
|
||
| def_node_matcher :gem_method?, <<-PATTERN | ||
| [(:send, nil, :gem, ...)] |
There was a problem hiding this comment.
Accidentally got four spaces here. Also would consider renaming the matcher to #gem_declaration?. 😀
| @@ -12,6 +12,12 @@ def inspect_source_file(cop, source) | |||
| Tempfile.open('tmp') { |f| inspect_source(cop, source, f) } | |||
| end | |||
|
|
|||
| def inspect_gemfile(cop, source) | |||
| Tempfile.open('tmp') do |_| | |||
There was a problem hiding this comment.
If the argument is not needed, can drop it?
There was a problem hiding this comment.
Yes. Apparently, I'm not using the Tempfile at all!
| end | ||
|
|
||
| def_node_matcher :gem_method?, <<-PATTERN | ||
| [(:send, nil, :gem, ...)] |
There was a problem hiding this comment.
I believe you can drop the brackets here. The brackets are for specifying multiple conditions which must all be true: node pattern documentation
| ) | ||
| end | ||
|
|
||
| def_node_matcher :gem_method?, <<-PATTERN |
There was a problem hiding this comment.
You can probably just use def_node_search here and drop the ast.each_descendant stuff above
There was a problem hiding this comment.
Neat!
Your example pattern seems to match the tests with or without the leading $. I'm not sure what the arbitrary matching on the capture means.
|
@jmks embrace the power of the def_node_search :gems, '$(send nil :gem str ...)'
def investigate(processed_source)
duplicated_gems =
gems(processed_source.ast)
.group_by { |node| node.method_args.first }
.select { |_, nodes| nodes.size > 1 }
.map { |_, nodes| nodes.first }
end |
|
|
||
| it "references gem's first occurance in message" do | ||
| inspect_gemfile(cop, source) | ||
| expect(cop.offenses.first.message).to include(2.to_s) |
There was a problem hiding this comment.
Wouldn't it make more sense to use '2' here?
|
Yeah, using a separate cop group (e.g. |
| end | ||
| end | ||
|
|
||
| def offense(node, gem_name, line_of_first_occurance) |
There was a problem hiding this comment.
I think proper spelling is occurrence.
|
@jmks ping :-) |
|
Sorry, got busy this week. I'll make changes in the next couple days. Thanks for all the great feedback! |
|
@jmks OK |
|
I've updated many of the issues raised, but still looking into:
|
Just add this to the |
We can think about this down the road I guess. |
The DuplicatedGem cop checks for duplicate gem entries in Gemfiles.
jmks
force-pushed
the
gemfile_gem_requirement_only_once_cop
branch
from
6b20abc
to
3ec5906
Compare
|
Updated!
A couple simple cases of eval_gemfile 'Gemfile.local'
# and
local_gemfile = 'Gemfile.local'
eval_gemfile local_gemfile if File.exist?(local_gemfile)But they could be added later like you suggest. |
jmks
changed the title
Lint/DuplicatedGem copBundler/DuplicatedGem cop
A part of #3600
This cop checks that gems are only listed once in a Gemfile.
I think bundler prints a warning if there's a duplicate, and only raises an error if there's a dependency issue (e.g. different versions). I don't know of a case when it's necessary to list a gem twice.
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).and description in grammatically correct, complete sentences.
The DuplicatedGem cop checks for duplicate gem entries in Gemfiles.